-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change remaining_bits_in_bitstream to remaining_bits_in_block #87
Conversation
612134a
to
400465c
Compare
Following conversation on CELLAR mailing list.
I agree on not redefining commonly used terms, but in that case I am in favor of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one nit comment, otherwise looks good
ffv1.md
Outdated
@@ -152,25 +152,26 @@ a = b, a += b, a -= b, a *= b | |||
|
|||
### Pseudo-code | |||
|
|||
Several components of FFV1 are described in this document using pseudo-code. Note that the pseudo-code is used for clarity in order to illustrate the structure of FFV1 and not intended to specify any particular implementation. The pseudo-code used is based upon the C programming language [@!ISO.9899.1990] as uses its `if/else`, `while` and `for` functions as well as functions defined within this document. | |||
FFV1 bitstream is described in this document using pseudo-code. Note that the pseudo-code is used for clarity in order to illustrate the structure of FFV1 and not intended to specify any particular implementation. The pseudo-code used is based upon the C programming language [@!ISO.9899.1990] as uses its `if/else`, `while` and `for` functions as well as functions defined within this document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer to start with The FFV1 bitstream is
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, my ugly English. Not the first time you do the remark if I remember well :).
Thanks, fixed.
Rebased |
Why is bitstream unclear ? |
In some other specs,
I think that the suggested change is better than current version, and could be improved in a future patch if someone else finds a better way.
I prefer |
(To me, |
I think using "packet" can lead to confusion in an environment where there are network packets. These would very likely be 2 different things |
I reviewed the Ogg specification at http://ietf.org/rfc/rfc3533.txt and think it may make sense to align with its use of Within RFC3533 it includes this paragraph in the definitions section:
And adds a caveat that addresses @michaelni's consideration of confusion with the term.
I propose keeping the use of |
I have a preference too for reusing same wording as in other RFC. |
4d31b08
to
ec53d5a
Compare
Reading again the OGG definitions part, difficult IMO to use it as is becaue it is the point of view of a container and not a coding format. I added the definition of Container and Packet, adapting the first part and keeping as is the part about avoid the confusion. Please review |
That’s my feeling too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
If you want to use terminology from an existing Specification then H264/H265/mpeg4/H222 access unit could be used. H264: |
I am ok with |
Why would it need to include the Configuration Record ? (That would be wrong for what an AU generally means) |
I presumed that the suggestion would change |
We could write "Access unit or Configuration Record" or something like that |
To confirm, you mean keep the term
? Seems fine. |
Yes, this is a possibility |
ec53d5a
to
5983d7b
Compare
Issue with that is that "Access Unit" becomes a synonym of "Frame", and we would have to change any instance of "Frame" word (huge). IIUC decision is to keep "bitstream" wording, so I changed of method:
I think it would clarify the meaning of "bitstream" while keeping this word, removing the concern about the usage of this word. Please review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few minor comments, but otherwise this looks good to me
ffv1.md
Outdated
@@ -26,6 +26,12 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S | |||
## Definitions | |||
|
|||
-------- -------------------------------------------------------------- | |||
`Container`: Format whose specification describes how packets and metadata coexist in a file. | |||
|
|||
`Configuration Record`: Encoder-created bitstream encapsulated in a container, representing a meaningful non visual element used for setting up the decoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could consider initialising the decoder
rather than setting up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-visual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meaning this is not a frame, no visual info encoded.
Better wording to suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, sorry: I have the feeling that a -
is necessary, but I may be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha. Ok I'll add the dash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added locally, will push to this branch soon
ffv1.md
Outdated
@@ -26,6 +26,12 @@ The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "S | |||
## Definitions | |||
|
|||
-------- -------------------------------------------------------------- | |||
`Container`: Format whose specification describes how packets and metadata coexist in a file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the remaining packets
here intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess typo, will change (frame and configuration record?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this locally, but am also rewording as Format whose specification describes how
is not true when Frames and Configuration Record are used with their FFV1 meansing; for instance ISOBMFF is a Container but its specification does not describe how to container the Configuration Record. Also 'file' is too specific as a Container could encapsulate FFV1 into a stream. Updating to:
Container
: Format that encapsulatesFrames
and (when required) aConfiguration Record
into a bitstream.
I made the changes suggested in the discussion here and added backtick quoting to some defined terms. Please re-review. |
Fine for me (I can't approve my own PR :) ) |
Circular definitions: And it sounds confusing |
In this case I suggest to remove the Configuration Record definition from the patch as it is well defined in its own section later in the document. |
7a6f78d
to
b3afed7
Compare
b3afed7
to
d1c41a9
Compare
I removed |
|
||
`Pixel`: The smallest addressable representation of a color in a frame. It is composed of 1 or more samples. | ||
`Sample`: The smallest addressable representation of a color component or a luma component in a `Frame`. Examples of sample are Luma, Blue Chrominance, Red Chrominance, Alpha, Red, Green, Blue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest a more puristic naming: Examples of sample are Luma, Blue minus Luma, Red minus Luma, Alpha, […]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the term as Jerome has it. I think the terms "blue minus luma / red minus luma" are misleading by oversimplifying a transform as the color space would likely add weights and constants. As examples "Blue Chrominance, Red Chrominance" would apply more broadly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not convinced, sorry! Yet, as said, for me this is not blocking at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the pedantic wording (not blocking!), otherwise LGTM.
bump on this, I think all concerns were handled, and the patch is small. |
"bitstream" wording is misleading because we want to check the end of the block (either ConfigurationRecord or Frame) and not the complete bitstream.
Issue reported by @dwbuiten